CASSANDRA-21574 Fix failing tests for TrackerReconcilerTest - #288
Conversation
|
|
||
| private boolean previousEpochForRegainedRangeRetired(Topology current, Ranges regainingRanges) | ||
| { | ||
| // In cases where nodeLookup is null, we are not testing invariants that are |
There was a problem hiding this comment.
don't feel this comment helps. It didn't really tell me anything so I had to walk the code to understand why its there.
What i see is that 2 code paths set null and one sets nodeMap:::get... The 2 code paths that set null do not create Node so we don't have the metadata needed to do this check...
We could have a comment like this?
When nodeLookup isn't defined we are unable to get node state, so assume that the calling test doesn't care about retired ranges
| case Success: | ||
| Assertions.assertTrue(tracker.all(FastPathShardTracker::hasReachedQuorum)); | ||
| Assertions.assertTrue(tracker.all(shard -> shard.fastPathIsRejected() || shard.hasMetFastPathCriteria() || shard.fastPathIsDelayed())); | ||
| Assertions.assertTrue(tracker.all(shard -> shard.hasMetFastPathCriteria() || (shard.fastPathFailures + shard.fastPathDelayed > 0) || shard.fastPathIsRejected() || shard.fastPathIsDelayed())); |
There was a problem hiding this comment.
| Assertions.assertTrue(tracker.all(shard -> shard.hasMetFastPathCriteria() || (shard.fastPathFailures + shard.fastPathDelayed > 0) || shard.fastPathIsRejected() || shard.fastPathIsDelayed())); | |
| Assertions.assertTrue(tracker.all(shard -> shard.hasMetFastPathCriteria() || shard.fastPathIsRejected() || shard.fastPathIsDelayed() || (shard.fastPathFailures + shard.fastPathDelayed > 0))); |
Reordering the assert order made it harder to validate what changed in the diff
There was a problem hiding this comment.
Can you also explain why (shard.fastPathFailures + shard.fastPathDelayed > 0)?
so we check: met fast path, fast path is rejected, fast path is delayed... so your check just says "at least 1 failure or delay exists"? can you explain the situation you found and why this is the right fix?
3 participants
Fixes tests for TrackerReconcilerTest and TopologyManagerTest::fuzz.